feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler by clintcan · Pull Request #1359 · Devolutions/IronRDP

clintcan · 2026-06-02T10:08:55Z

Implements the misuse-resistant RdpsndServerHandler format-negotiation API discussed in #1356, in the two-method shape you proposed there.

Problem

The old start(&ClientAudioFormatPdu) -> Option<u16> made every handler compute wFormatNo itself — an index into the client's format list. Returning the wrong index (e.g. a server-list index) yields wFormatNo >= NumberOfClientFormats, which a compliant client rejects, silently dropping all audio. Every handler also re-implemented the same server∩client intersection. (Documented in #1343.)

Change

Move the negotiation into the crate and split selection from lifecycle:

fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>;
fn start(&mut self, format: &NegotiatedFormat);

The crate computes common — the formats from get_formats() the client also advertised, in the server's preference order, each tagged with its client-list wFormatNo — and calls choose_format, then start with the chosen format.
NegotiatedFormat has a private wformat_no and no public constructor, and choose_format returns a borrow of an element of common, so a handler cannot pick a format the client didn't accept nor emit an out-of-range wFormatNo.
choose_format is not called when nothing is in common.
choose_format (pure selection) and start (encoder init / producer startup) are kept distinct, per your note that start is also where producers initialize.

Resolves the footgun in #1343. Closes #1356.

Two decisions worth a look (easy to change)

NegotiatedFormat.format is private with a format() accessor rather than pub format — clippy::partial_pub_fields rejects mixing a pub field with the private wformat_no, and this matches the direction of fix(egfx)!: make DecodedFrame fields private with getters to enforce size invariant #1331. Happy to switch to pub + #[allow] if you prefer.
Format matching uses WAVEFORMATEX identity (audio_format_eq: wave-format tag, channels, sample rate, bit depth), deliberately ignoring derived fields (n_avg_bytes_per_sec, n_block_align) and the codec data blob, which a client may not echo byte-for-byte. This is looser than the type's derived ==. I went with identity matching because strict equality can miss formats a client genuinely supports (→ silent no-audio, the very bug this targets), but if you'd rather the crate use full ==, it's a one-line change.

Notes

Breaking change (pre-v1): replaces RdpsndServerHandler::start. Known downstream implementors that will need a small update: lamco-rdp-server, hypr-rdp, cosmic-ext-rdp-server, ARISU, macrdp. (Per the rdpsnd: make RdpsndServerHandler::start misuse-resistant (hand it the common formats, let the crate compute wFormatNo) #1356 discussion, the straight replacement was preferred over an additive/deprecated path.)
The bundled example server is updated to the new shape.

Testing

New unit tests for negotiate_formats / audio_format_eq: client-index mapping, the PCM-only/AAC-first regression, empty overlap, and identity equality. (Enabled the lib test harness in Cargo.toml.)
cargo clippy -p ironrdp-rdpsnd --all-targets -- -D warnings and the example (--features cliprdr,connector,rdpsnd,server) are clean; cargo test -p ironrdp-rdpsnd passes.

clintcan · 2026-06-26T12:07:08Z

Friendly ping on this when you have a moment 🙂 — it's been a few weeks and I wanted to make sure it didn't slip off the radar.

This implements the exact two-method shape we settled on in #1356: choose_format(&mut self, common: &[NegotiatedFormat]) -> Option<&NegotiatedFormat> for pure selection + start(&mut self, format: &NegotiatedFormat) as the lifecycle hook, with the crate owning the wFormatNo computation/validation. CI is green and it's currently mergeable (no conflicts).

Two small in-PR points I'd flagged for your call are still open whenever you review: (1) the format() accessor vs a pub format field (clippy partial_pub_fields / #1331), and (2) audio_format_eq doing a WAVEFORMATEX-identity match vs a full ==. Happy to adjust either, and glad to rebase if it drifts before you get to it. No rush — just flagging it's ready.

Benoît Cortier (CBenoit) · 2026-06-29T13:08:13Z

Thank you! I added this PR to my review queue! I’m requesting a Copilot review in the meantime.

Copilot

Pull request overview

This PR updates the ironrdp-rdpsnd server-side handler API to make audio format negotiation misuse-resistant by moving the server∩client intersection and wFormatNo (client-list index) computation into the crate, and splitting “selection” from “lifecycle start”. The bundled ironrdp example server is updated accordingly, and unit tests are added/enabled to validate the negotiation logic.

Changes:

Introduces NegotiatedFormat and replaces RdpsndServerHandler::start(&ClientAudioFormatPdu) -> Option<u16> with choose_format(&[NegotiatedFormat]) -> Option<&NegotiatedFormat> plus start(&NegotiatedFormat).
Adds crate-owned negotiation helpers (negotiate_formats, audio_format_eq) and unit tests covering index mapping and overlap behavior.
Updates crates/ironrdp/examples/server.rs to use the new two-step negotiation API.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
crates/ironrdp/examples/server.rs	Updates the example RDPSND handler implementation to the new `choose_format` + `start` API.
crates/ironrdp-rdpsnd/src/server.rs	Adds `NegotiatedFormat`, negotiation logic, handler API change, and unit tests.
crates/ironrdp-rdpsnd/Cargo.toml	Enables the lib test harness so the new unit tests run.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+                    // `chosen` borrows `common` (not `self`), so the encoder
+                    // is read off it and the handler is free to borrow again
+                    // for the `start` lifecycle hook.
+                    let wformat_no = chosen.wformat_no;
+                    self.handler.start(chosen);


Benoît Cortier (CBenoit)

Finally went through it! Thank you, I think this is the right direction, and  I love the new API.

Benoît Cortier (CBenoit) · 2026-06-30T16:43:26Z

                    warn!("Invalid OPUS channels: {}", n);
-                    return Some(0);
+                    return;


issue: start() now returns (). So format_no is committed before start runs, and start has no way to say "I couldn't initialize." We can see in this example where the encoder creation fails and we do a bare return, after which the library still sets format_no = Some(wformat_no) and considers the stream live. The problem is that at this point no producer task was spawned, and we have a silent "negotiated, no audio" state, which is a failure class I wish we could kill in this PR. I think we need to either return Result<(), E> or a bool so the library can roll back to None.

Benoît Cortier (CBenoit) · 2026-06-30T16:54:40Z

 [lib]
 doctest = false
-test = false
+# test = false


issue: test = false is deliberate policy here, not incidental, let's not re-enable it.

Keeping the lib free of an inline test harness is a constraint we hold across the workspace, and a commented-out manifest key will just rot. The behavior is well worth testing though. Here is a route that keeps the policy intact:

add an __internal feature (doc-hidden, excluded from default),

behind it, expose thin #[doc(hidden)] pub shims over the private items: negotiate_formats, audio_format_eq, and a wformat_no() accessor on NegotiatedFormat (the current tests read the private wformat_no field directly, which only works because they live in-module, so the shim has to surface that too),

move the tests into the appropriate testsuite crate

[lib] test = false stays, the internal surface stays out of the public API and docs, and the assertions you already wrote port over almost verbatim.

Benoît Cortier (CBenoit) · 2026-06-30T16:56:17Z

+/// Compare two audio formats by their WAVEFORMATEX identity — wave format tag,
+/// channel count, sample rate, and bit depth. Derived fields
+/// (`n_avg_bytes_per_sec`, `n_block_align`) and the codec-specific `data` blob
+/// are deliberately ignored: a client echoes back a format it accepts but is
+/// not guaranteed to reproduce those byte-for-byte.
+fn audio_format_eq(a: &pdu::AudioFormat, b: &pdu::AudioFormat) -> bool {
+    a.format == b.format
+        && a.n_channels == b.n_channels
+        && a.n_samples_per_sec == b.n_samples_per_sec
+        && a.bits_per_sample == b.bits_per_sample
+}


issue: ignoring the codec data blob is unsafe for some formats we actually advertise.

Agreed on the derived fields (n_avg_bytes_per_sec / n_block_align are computable and a client may legitimately not echo them), but the data blob is a different category: for codecs whose extra-format bytes carry real configuration (AAC is the clear case per MS-RDPEA 2.2.2.1.1's cbSize/extra data, worth confirming per codec in our get_formats), blanket-ignoring it can match two genuinely incompatible variants.

Concretely, with the current looseness plus format: server_format.clone() in negotiate_formats: we stream the server's definition while wFormatNo points the client at its entry. If they differ only in data, the server encodes one config and the client decodes another, garbled/no audio. Two distinct server formats differing only in data also collapse onto the same client index, yielding two NegotiatedFormats sharing one wFormatNo.

PCM is fine (no extra data); the codecs that aren't need either codec-aware matching or data included in the comparison for those. Note equality_uses_waveformatex_identity_only currently encodes the too-loose decision, so it'd flip with this.

Benoît Cortier (CBenoit) · 2026-06-30T16:58:07Z

+                // Formats common to server and client, in the server's
+                // preference order, each tagged with its wFormatNo (its
+                // position in the *client's* list). Keeping this in the crate
+                // means the handler never does index arithmetic and can't emit
+                // an out-of-range wFormatNo.
+                let common = negotiate_formats(self.handler.get_formats(), &client_format.formats);
                self.state = RdpsndState::Ready;
-                self.format_no = self.handler.start(client_format);
+                self.format_no = if common.is_empty() {
+                    debug!("No audio format in common with the client; audio disabled");
+                    None
+                } else if let Some(chosen) = self.handler.choose_format(&common) {
+                    // `chosen` borrows `common` (not `self`), so the encoder
+                    // is read off it and the handler is free to borrow again
+                    // for the `start` lifecycle hook.
+                    let wformat_no = chosen.wformat_no;
+                    self.handler.start(chosen);
+                    Some(wformat_no)
+                } else {
+                    debug!("Handler declined every common audio format; audio disabled");
+                    None
+                };


thought: I would value a test on the SvcProcessor wiring over the isolated helper tests; but fine as a follow-up, or I may pick it up myself.

The helpers are well covered; the part that's new and easy to get wrong is the state-machine path in process: choose_format skipped when common is empty, decline -> None, the captured wformat_no matching the chosen format, start called exactly once. A black-box test: drive the processor with a Client Audio Formats PDU through a fake handler, emit a wave, assert the wFormatNo on the outgoing PDU; it would exercises all of that through the public surface, so it would live as a normal tests/ integration test and wouldn't even need the __internal shim from the Cargo.toml thread.

Not blocking: happy to take it as a follow-up PR, or leave it 🙂

…are codec data Review follow-up for Devolutions#1359. - `start` now returns `Result<(), Box<dyn RdpsndError>>`. On `Err` the crate declines the negotiated format (clears `format_no`) — the same outcome as `choose_format` returning `None` — instead of leaving the channel "negotiated" but silently producing no audio. `format_no` is committed *before* `start` so a producer that emits a wave during `start` can't race an unset index, and is rolled back to `None` on failure. The example server's two encoder-init failures now return `Err`. - Restore `[lib] test = false` (workspace policy) and move the negotiation tests out of the lib. Add a private `__test` feature (`dep:visibility`) exposing `negotiate_formats` / `audio_format_eq` / `NegotiatedFormat::wformat_no` via `visibility::make(pub)`; the tests live in `ironrdp-testsuite-core` (`tests/rdpsnd/server.rs`), which depends on `ironrdp-rdpsnd` with `__test` — mirroring `ironrdp-cliprdr`. - `audio_format_eq` now compares the codec extra-data blob (`data`). The two derived fields (`n_avg_bytes_per_sec`, `n_block_align`) stay ignored, but the `data` blob carries real configuration for some codecs (AAC's HEAACWAVEINFO, MS-RDPEA 2.2.2.1.1), so ignoring it could match two genuinely incompatible formats. cbSize=0 still decodes to `None` on both sides, so PCM/AAC negotiation is unaffected. - Add black-box `SvcProcessor` wiring tests (via the public surface): a shared format streams (start called once, format committed), no common format skips `choose_format`, and a failing `start` declines (no audio streamed).

clintcan · 2026-06-30T19:15:35Z

Thanks for the thorough review Benoît Cortier (@CBenoit)! Pushed a follow-up addressing all of it:

start() can now fail (your examples/server.rs:299 + Copilot's server.rs:318). start returns Result<(), Box<dyn RdpsndError>>. The crate commits format_no before calling start (so a producer that emits a wave during start can't race an unset index), and on Err rolls it back to None — declining the format exactly as choose_format returning None would, instead of the silent "negotiated, no audio" state. The example's two encoder-init failures now return Err(...).

test = false kept (your Cargo.toml:17). Restored test = false, added a private feature that exposes negotiate_formats / audio_format_eq and a wformat_no() accessor on NegotiatedFormat, and moved the tests into ironrdp-testsuite-core (tests/rdpsnd/server.rs). One deviation from your suggestion: I named the feature __test and used visibility::make(pub) rather than __internal + hand-written shims — that's the existing workspace idiom (ironrdp-cliprdr does exactly this, ironrdp-testsuite-core already pulls crates in via __test, and __test is in xtask's EXCLUDE_FEATURES so the feature-matrix skips it; __internal wouldn't be excluded). Same outcome — doc-hidden, out of the public API/docs. Happy to rename if you'd prefer __internal.

data is now compared (your server.rs:254). audio_format_eq compares the extra-data blob; the two derived fields stay ignored. cbSize=0 decodes to None on both sides (pdu/mod.rs decode), so PCM/AAC are unaffected, while AAC variants that differ only in HEAACWAVEINFO no longer collapse. The old equality_uses_waveformatex_identity_only test flipped as you predicted (now equality_ignores_derived_fields_but_not_extra_data), plus a new extra_data_must_match_for_otherwise_identical_formats.

SvcProcessor wiring test (your server.rs:323). Went ahead and added it — black-box through the public surface (no __test shim), driving a fake handler: a shared format streams (start called exactly once, format_no committed), no common format skips choose_format, and a failing start declines (no wave streamed) — which also covers the new roll-back path.

Green locally: cargo fmt --all -- --check, cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings, and cargo test --workspace --locked all pass (also verified rdpsnd builds + clippies clean both with and without __test).

…ndler The old `start(&ClientAudioFormatPdu) -> Option<u16>` made every handler compute `wFormatNo` itself, as an index into the *client's* format list. Getting it wrong (e.g. returning a server-list index) yields `wFormatNo >= NumberOfClientFormats`, which a compliant client rejects, silently dropping all audio — and each handler re-implemented the same server-vs-client intersection. Move that work into the crate and split selection from lifecycle: fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>; fn start(&mut self, format: &NegotiatedFormat); The crate computes `common` (formats from `get_formats()` the client also advertised, in the server's preference order, each tagged with its client-list `wFormatNo`), calls `choose_format`, then `start` with the chosen format. `NegotiatedFormat` has a private `wformat_no` and no public constructor, and `choose_format` returns a borrow of an element of `common`, so a handler cannot pick a format the client did not accept nor emit an out-of-range `wFormatNo`. `choose_format` is not called when nothing is in common. Separating `choose_format` (pure selection) from `start` (encoder init / producer startup) keeps the two concerns distinct. Resolves the footgun documented in Devolutions#1343. Closes Devolutions#1356. BREAKING CHANGE: `RdpsndServerHandler::start` is replaced by `choose_format` (selection) plus `start(&NegotiatedFormat)` (lifecycle). Implementors now return a `&NegotiatedFormat` from `choose_format` instead of computing a `wFormatNo`. - Add `NegotiatedFormat` (private `wformat_no`, `format()` accessor) and `negotiate_formats` (intersection + client-index mapping), with unit tests for client-index mapping, the PCM-only/AAC-first regression, empty overlap, and WAVEFORMATEX-identity equality. - Match formats on WAVEFORMATEX identity (tag/channels/rate/bits), ignoring derived and codec-`data` fields a client may not echo verbatim. - Update the example server to the new two-method shape.

…are codec data Review follow-up for Devolutions#1359. - `start` now returns `Result<(), Box<dyn RdpsndError>>`. On `Err` the crate declines the negotiated format (clears `format_no`) — the same outcome as `choose_format` returning `None` — instead of leaving the channel "negotiated" but silently producing no audio. `format_no` is committed *before* `start` so a producer that emits a wave during `start` can't race an unset index, and is rolled back to `None` on failure. The example server's two encoder-init failures now return `Err`. - Restore `[lib] test = false` (workspace policy) and move the negotiation tests out of the lib. Add a private `__test` feature (`dep:visibility`) exposing `negotiate_formats` / `audio_format_eq` / `NegotiatedFormat::wformat_no` via `visibility::make(pub)`; the tests live in `ironrdp-testsuite-core` (`tests/rdpsnd/server.rs`), which depends on `ironrdp-rdpsnd` with `__test` — mirroring `ironrdp-cliprdr`. - `audio_format_eq` now compares the codec extra-data blob (`data`). The two derived fields (`n_avg_bytes_per_sec`, `n_block_align`) stay ignored, but the `data` blob carries real configuration for some codecs (AAC's HEAACWAVEINFO, MS-RDPEA 2.2.2.1.1), so ignoring it could match two genuinely incompatible formats. cbSize=0 still decodes to `None` on both sides, so PCM/AAC negotiation is unaffected. - Add black-box `SvcProcessor` wiring tests (via the public surface): a shared format streams (start called once, format committed), no common format skips `choose_format`, and a failing `start` declines (no audio streamed).

clintcan force-pushed the feat-rdpsnd-negotiated-format branch from f6e94a5 to 72d28d2 Compare June 2, 2026 10:31

clintcan mentioned this pull request Jun 2, 2026

rdpsnd: make RdpsndServerHandler::start misuse-resistant (hand it the common formats, let the crate compute wFormatNo) #1356

Open

clintcan force-pushed the feat-rdpsnd-negotiated-format branch from 72d28d2 to d657272 Compare June 28, 2026 04:03

Benoît Cortier (CBenoit) requested a review from Copilot June 29, 2026 13:07

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) June 29, 2026 13:07 View session

Copilot AI reviewed Jun 29, 2026

View reviewed changes

Benoît Cortier (CBenoit) reviewed Jun 30, 2026

View reviewed changes

Clint Christopher Canada added 2 commits July 1, 2026 03:18

clintcan force-pushed the feat-rdpsnd-negotiated-format branch from 1a4b6ec to b0783e4 Compare June 30, 2026 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359

feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359
clintcan wants to merge 2 commits into
Devolutions:masterfrom
clintcan:feat-rdpsnd-negotiated-format

clintcan commented Jun 2, 2026

Uh oh!

clintcan commented Jun 26, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Jun 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Benoît Cortier (CBenoit) left a comment •

edited

Loading

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Uh oh!

clintcan commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

clintcan commented Jun 2, 2026

Problem

Change

Two decisions worth a look (easy to change)

Notes

Testing

Uh oh!

clintcan commented Jun 26, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Jun 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Benoît Cortier (CBenoit) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

clintcan commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Benoît Cortier (CBenoit) left a comment •

edited

Loading